Code Review

Overall this is a well-motivated pair of fixes with clear comments and good separation of infra vs. per-iteration failures. A few items worth addressing before merge.

1. No test coverage for the nonce desync recovery path (builder.go)

The most significant new logic — the mempool inspection + conditional ResetNonce call — has no automated test. The test plan says go test ./pkg/erc4337/preset/... but that would only catch regressions in existing tests; the three distinct cases in the new branch are all untested:

• (a) mempool has a predecessor → keep current nonce, sleep + retry
• (b) mempool is empty → rewind to on-chain nonce via ResetNonce
• (c) mempool API returns an error → conservative fallback (predecessorPending = true)

Since BundlerClient is a concrete struct (not an interface), you'd need either an interface + mock or an integration test to cover this. Given that this path is exercised only on a live bundler, at minimum a comment explaining what test infrastructure is missing and tracking it as a follow-up would be appropriate.

2. context.Background() with no timeout for mempool inspection (builder.go:1169)

if pendingOps, mempoolErr := bundlerClient.GetPendingUserOpsForSender(context.Background(), entrypoint, userOp.Sender); ...

All other bundlerClient calls in this function already use context.Background(), so this is consistent with the existing pattern — but it's still a concern. If the bundler's RPC hangs on GetPendingUserOpsForSender, the entire retry loop stalls indefinitely on this new call. The existing calls (gas estimation, send) are at least on the hot path where a hang is obvious; this one is in a recovery branch that's harder to observe.

Suggestion: use a short-lived derived context:

mempoolCtx, mempoolCancel := context.WithTimeout(context.Background(), 5*time.Second)
defer mempoolCancel()
pendingOps, mempoolErr := bundlerClient.GetPendingUserOpsForSender(mempoolCtx, entrypoint, userOp.Sender)

3. Silent skip on unparseable nonce with no debug log (builder.go:1172-1179)

if _, ok := opNonce.SetString(strings.TrimPrefix(op.Nonce, "0x"), 16); !ok {
    if _, ok := opNonce.SetString(op.Nonce, 10); !ok {
        continue  // silently skipped
    }
}

If a bundler returns a nonce in an unexpected format, it's silently ignored and predecessorPending stays false, which would trigger a spurious ResetNonce. Add a debug log before continue:

l.Debug("failed to parse pending op nonce, skipping", "nonce", op.Nonce, "sender", userOp.Sender.Hex())

4. Nonce parse ambiguity: decimal-looking strings are silently reinterpreted as hex

big.Int.SetString(strings.TrimPrefix("10", "0x"), 16) succeeds — it parses "10" as hex 16, not decimal 10. The decimal fallback is never reached for bare digit strings. In practice EIP-4337 bundlers return "0x..." hex strings so this path won't be hit, but the comment on the fallback block (// Fall back to decimal parse) implies it handles decimal — it only handles strings that are invalid hex. Worth a clarifying comment or just removing the fallback if the only realistic input is "0x...".

5. success at finalizeStep is always true (vm.go:4853)

After the change, success is set to false only in the same places that also set infraFailure = true. The early-return at line 4848 handles every case where success == false. So finalizeStep(s, success, ...) at line 4853 is always equivalent to finalizeStep(s, true, ...).

This isn't wrong, but it's subtly misleading — it looks like success could be false here. Consider making the invariant explicit:

// infraFailure cases already returned above; success is always true here.
finalizeStep(s, true, nil, "", log.String())

6. Test assertion gap in TestLoopNode_ContractWrite_InvalidAddress_PartialFailure

The test now asserts step.Success == true (correct) but drops the previous step.Error assertion without adding a replacement. Worth explicitly asserting step.Error == "" to lock in the expected behavior that a partial-success loop does not surface an error string:

assert.Empty(t, step.Error, "Loop step error should be empty when loop ran to completion")

Nit: target branch

CLAUDE.md says feature branches target staging, never main directly — but this PR is the staging → main release PR, which is the documented release flow. No action needed; just noting it's expected.

Summary

The logic is sound — items 1 and 2 are the ones I'd prioritize before shipping.